-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bright Neighbor Validation #2
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2 +/- ##
=======================================
- Coverage 95.2% 88.7% -6.5%
=======================================
Files 8 9 +1
Lines 327 351 +24
Branches 64 66 +2
=======================================
Hits 311 311
- Misses 4 28 +24
Partials 12 12 ☔ View full report in Codecov by Sentry. |
I have now added offsetting and proper motion correction to the bright neighbor check. A couple things that have changed as a result of this:
One other thing to figure out. The offsetting function relies on knowing what observatory the observation will be at. Is this something we will know at the validation stage? Or is it simpler to just run the validation for each observatory? |
Hi @imedan, I have finally gotten to a point testing the validation that I think I have some relevant comments. Instead of merging this branch directly I did merge/copy-paste/modify your additions in this file in the https://github.com/sdss/too/blob/dev/src/too/validate.py The main reason for this is that I wanted to avoid depending on Mugatu for now, as it requires some additional dependencies to build Kaiju and it only works with Python <= 3.10 (which is what Kaiju allows). Happy to revisit that if we can make Mugatu to not depend on Kaiju and work with the latest Pythons. In any case, what I did was to copy some functions from Mugatu and a trimmed down version of
The non-critical but useful change I made is to use Currently the version in Let me know what you think and whether we should just merge those changes to |
So, I think it does make sense (for now) to just copy over the Other than this, the changes that you made look all good to me! For the two errors you caught, I did correct those locally but forgot to push them to the remote branch...so thanks for catching them! Also, the I also looked at your implementation of validating the targets. It looks mostly good to me. I guess it is still missing differentiating between the observatories for the targets? This is a change we will have to make, as the bright neighbor radius is not the same at APO and LCO. Other than that last comment, this looks good to merge to |
Sounds good, thanks for looking at the That reminds me that I wanted to ask about a detail of how the function should be called. Is the idea (as I wrote it right now) that we check every target for all the design modes that match their bright/dark sky brightness mode, and that if any fail we reject that target? Or was your idea that we only check the design mode of the design in which the too target is replacing a science target? I think the latter cannot be done except on the mountain. |
Yes, I think this would make the most sense and we should be able to do this ahead of time. Should I add a function to
This is close to my intention. You are right that you need to check every target for the design modes that match their bright/dark sky brightness mode (so if "bright", only need to check design modes with "bright" in them). The second half isn't quite correct though. For example, it is possible that a ToO could be valid in dark_plane, but not dark_faint. I would imagine the intention should be that the ToO could be observed in dark_plane designs, but not dark_faint ones. So, I really think we need to store the validation results for each target and for each design mode. Then, on the mountain for a specific design we should be able to select 1) targets that are within the FOV of the field and 2) targets that are valid for the design mode for the specific design. Does that sound right to you? |
Ah yes, I had not thought of storing those results. That makes total sense. |
This is all integrated in the |
@albireox I wanted to start a draft of a pull request so we can comment on what I have so far for the bright neighbor validation. The current implementation relies on having the bright neighbor healpix indices saved in a file that are pointed to by the environment variable
BN_HEALPIX
. Then it just uses the R.A. and Decl. of the ToOs to see if they are in the bright neighbor regions on the sky for a givendesign_mode
. Currently, for the sample filedocs/sample.csv
, the check for onedesign_mode
takes ~1 minute.A couple things that may need to be added are:
Let me know what you think.